Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Add --path option support for artisan migrate:rollback/refresh/reset fixes #13631 #15251

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

anderly
Copy link
Contributor

@anderly anderly commented Sep 3, 2016

Added the --path command option to the RollbackCommand, RefreshCommand and ResetCommand to support rolling back commands that may have been executed with the --path option in the MigrateCommand. This is especially needed for multitenant db migrations where tenant database migrations are typically stored in a tenant subfolder of the migrations directory.

More background:

I have a multi-tenant app with one main application/security database and separate tenant databases. I keep tenant migrations stored in a migrations/tenant subfolder. I have a custom Artisan command (php artisan migrate:tenant) which, in turn, calls php artisan migrate for each tenant and passes the --database=tenant option to specify the tenant database connection and the --path option to specify the path to the tenant migrations (i.e. database/migrations/tenant). This works fine and executes the migrations against the tenant database(s), but if I try to run php artisan migrate:rollback --database=tenant, it tries to rollback the migrations in the main migrations folder, not the migrations/tenant subfolder. So, the --path option is needed on the rollback, refresh and reset commands in order to make this work.

@anderly anderly changed the title Add path support for artisan migrate:rollback [5.3] Add path support for artisan migrate:rollback Sep 3, 2016
@anderly anderly changed the title [5.3] Add path support for artisan migrate:rollback [5.3] Add --path option support for artisan migrate:rollback Sep 3, 2016
@anderly anderly changed the title [5.3] Add --path option support for artisan migrate:rollback Add --path option support for artisan migrate:rollback Sep 3, 2016
@anderly anderly changed the title Add --path option support for artisan migrate:rollback [5.3] Add --path option support for artisan migrate:rollback Sep 3, 2016
@srmklive
Copy link
Contributor

srmklive commented Sep 3, 2016

This won't work in my opinion. You may be providing the path, but it would still load all the migrations path.

@anderly
Copy link
Contributor Author

anderly commented Sep 3, 2016

I have tested this change and it indeed works with 5.3.x as the Illuminate\Database\Console\Migrations\BaseCommand respects the path option (if present) in the getMigrationPaths method. With this change in place, when I run migrate:rollback and specify a --path=database/migrations/tenant option it only attempts to rollback the migrations in the tenant subfolder and doesn't attempt to rollback the migrations in the main migrations folder.

Let me know if you have any questions.

@afigienas
Copy link

Also doesn't work --path option in migrate:refresh and migrate:reset

@anderly
Copy link
Contributor Author

anderly commented Sep 5, 2016

@afigienas looks like the RefreshCommand has the path option but the ResetCommand doesn't.

@afigienas
Copy link

@anderly but looks like $path variable are not sent to rollback or reset command.

https://github.com/laravel/framework/blob/5.3/src/Illuminate/Database/Console/Migrations/RefreshCommand.php


        if ($step > 0) {
            $this->call('migrate:rollback', [
                '--database' => $database, '--force' => $force, '--step' => $step,
            ]);
        } else {
            $this->call('migrate:reset', [
                '--database' => $database, '--force' => $force,
            ]);
        }

@afigienas
Copy link

it is used only in

  $this->call('migrate', [
            '--database' => $database,
            '--force' => $force,
            '--path' => $path,
        ]);

so the error comes in "clear DB" step

@anderly
Copy link
Contributor Author

anderly commented Sep 5, 2016

@afigienas oh, you're right! I only saw that the path option was defined.

Ok, so looks like the RefreshCommand and ResetCommand should respect and send the --path option when fired as well for this to be a complete patch.

I'll add those changes to this PR.

@anderly anderly changed the title [5.3] Add --path option support for artisan migrate:rollback [5.3] Add --path option support for artisan migrate:rollback/refresh/reset Sep 6, 2016
…aravel#13631

Added path support for artisan migrate:rollback/refresh/reset fixed laravel#13631

Add --path support to ResetCommand and RefreshCommand

Add --path to refresh command test.

Update refresh command test.
@anderly anderly force-pushed the migrate-rollback-path-support branch from 04c70c0 to 665ab29 Compare September 6, 2016 23:35
@anderly anderly changed the title [5.3] Add --path option support for artisan migrate:rollback/refresh/reset [5.3] Add --path option support for artisan migrate:rollback/refresh/reset fixes #13631 Sep 6, 2016
@anderly
Copy link
Contributor Author

anderly commented Sep 6, 2016

Updated RefreshCommand and ResetCommand, updated tests and squashed commits. Should be ready to go.

@taylorotwell taylorotwell merged commit b8b57f1 into laravel:5.3 Sep 7, 2016
@anderly anderly deleted the migrate-rollback-path-support branch September 8, 2016 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants